Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sixel Support #2157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Sixel Support #2157

wants to merge 1 commit into from

Conversation

theCapypara
Copy link

This adds support for displaying Sixels, in case VTE is compiled with support for it enabled.
This is related to #1806.

Upstream issue for Sixel support: https://gitlab.gnome.org/GNOME/vte/-/issues/253

Side notes:
While developing I came across three issues:

@Davidy22
Copy link
Collaborator

Sorry about the delay, I've dropped some older unsupported python versions from CI so it should run if you push a superficial change and I'll take a look in the meantime

Related to Guake#1806

sem-ver: feature
@@ -211,6 +211,15 @@ def configure_terminal(self):
if hasattr(self, "set_alternate_screen_scroll"):
self.set_alternate_screen_scroll(True)

enable_sixel = client.get_boolean("use-sixel")
Copy link
Collaborator

@Davidy22 Davidy22 Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a preferences option? Seems like the kind of thing that anyone who has a terminal that can support it would want. If we do keep this as an option in preferences, we also need to add some switches on setting toggle, because as-written currently if a user clicks the box to enable they still won't see sixels until they restart guake.

I'm leaning towards always on and throw a low log message if the toggle fails just because I feel like everyone who knows these exist would be fine having them. Preferences windows become scary when there's too many things in them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it's probably a good idea to have this optional at least as long as sixel support is still considered experimental in vte.

Copy link
Collaborator

@Davidy22 Davidy22 Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is experimental right now, it's also quite unavailable, I think the only way currently for people to have a version of general VTE that supports sixels installed is to compile it yourself, which is tantamount to opting in, and when it lands in an official VTE release pushed to general users I would think it is no longer experimental.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I'll remove the option to enable them and just turn them on when available.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably still keep the setting in the gschema, if advanced users want to still opt-out somehow / for some reason. But I'll default it to enabled, would that be okay?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, sure. And a memo for another day to change some things when sixels come to VTE proper.

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been busy, but I got the time to take a look at this, don't have a sixel enabled version of VTE installed to check that the flag works so I still need to do that but I do still have thoughts on parts of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants